include: devicetree: Add port and endpoint macros #80649

ngphibang · 2024-10-30T21:35:50Z

This PR adds some port / endpoint DT helpers for the video drivers or others (e.g. display) and some examples of their simple usage in video drivers. Two main macros helpers are

DT_INST_ENDPOINT_BY_ID(inst, pid, id) : to get the (local) endpoint node id from a port id and an endpoint id
DT_NODE_REMOTE_DEVICE(ep) : to get the peer remote device node from the local endpoint node

A typical usage would be parsing all endpoints in a loop, or automatically parsing all endpoints till the end.

josuah

It looks typical to have both DT_XXX(inst) and DT_INST_XXX(inst) variants defined, but not sure if the non-INST_ would ever be used.

#define DT_INST_XXX(inst) DT_XXX(DT_DRV_INST(inst))

Glad to offer the commit/review cycles it takes to get this ready for <zephyr/drivers/video.h> or <zephyr/devicetree.h>.

drivers/video/video_mcux_csi.c

drivers/video/video_mcux_mipi_csi2rx.c

josuah · 2024-10-31T11:46:06Z

include/zephyr/drivers/video.h

+#define _DT_INST_PORT_BY_ID(n, pid)                                                                \
+	COND_CODE_1(DT_NODE_EXISTS(DT_INST_CHILD(n, ports)), (DT_CHILD(DT_INST_CHILD(n, ports), port_##pid)), (DT_INST_CHILD(n, port_##pid)))


This seems like an ongoing discussion: #58007
But the status quo seems still be on having Z_ for internal macros for now:

zephyr/include/zephyr/sys/util_internal.h

Line 28 in 124aae3

#define Z_IS_ENABLED1(config_macro) Z_IS_ENABLED2(_XXXX##config_macro)

Sorry if I'm missing something here, why not use this pattern? Or remove the underscore if users should be using this macro directly?

We can use this pattern directly if we are sure that the target port has an ID, e.g. port_0, port_1, etc. It will not work in case we have only 1 port with no ID. I will remove the underscore as this macro can be used both internally and publicly.

include/zephyr/drivers/video.h

josuah · 2024-11-02T21:19:47Z

I tried to summarize my feedback into something like this, which I hope still matches what you had in mind:

/* Handle the variability of "ports{port@0{}};" vs "port{};" while going down */
#define DT_INST_PORT_BY_ID(inst, pid)                                                              \
	COND_CODE_1(DT_NODE_EXISTS(DT_INST_CHILD(inst, ports)),                                    \
		    (DT_CHILD(DT_INST_CHILD(inst, ports), port_##pid)),                            \
		    (DT_INST_CHILD(inst, port)))

/* Handle the variability of "endpoint@0{};" vs "endpoint{};" while going down */
#define DT_INST_ENDPOINT_BY_ID(inst, pid, eid)                                                     \
	COND_CODE_1(DT_NODE_EXISTS(DT_CHILD(DT_INST_PORT_BY_ID(inst, pid), endpoint)),             \
		    (DT_CHILD(DT_INST_PORT_BY_ID(inst, pid), endpoint)),                           \
		    (DT_CHILD(DT_INST_PORT_BY_ID(inst, pid), endpoint_##eid)))

/* Handle the variability of "ports{port@0{}};" vs "port{};" while going up */
#define DT_ENDPOINT_PARENT_DEVICE(node)                                                            \
	COND_CODE_1(DT_NODE_EXISTS(DT_CHILD(DT_GPARENT(node), port)),                              \
		    (DT_GPARENT(node)), (DT_PARENT(DT_GPARENT(node))))

/* Handle the "remote-endpoint-label" */
#define DEVICE_DT_GET_REMOTE_DEVICE(node)                                                          \
	DEVICE_DT_GET(DT_ENDPOINT_PARENT_DEVICE(                                                   \
		DT_NODELABEL(DT_STRING_TOKEN(node, remote_endpoint_label))))

Feel free to cherry-pick from it or ignore it altogether.

ngphibang · 2024-11-05T00:45:38Z

/* Handle the variability of "ports{port@0{}};" vs "port{};" while going down */
#define DT_INST_PORT_BY_ID(inst, pid)                                                              \
	COND_CODE_1(DT_NODE_EXISTS(DT_INST_CHILD(inst, ports)),                                    \
		    (DT_CHILD(DT_INST_CHILD(inst, ports), port_##pid)),                            \
		    (DT_INST_CHILD(inst, port)))

=> This does not work when there is no ports node but we still have multiple ports, each has a pid (they are not grouped under a ports node though). Note that the ports node is optional.

That's why I defined two macros so that it works for all cases.

/* Drivers should not use this except they know exactly that port has a pid */
#define _DT_INST_PORT_BY_ID(n, pid)                                                                \
	COND_CODE_1(DT_NODE_EXISTS(DT_INST_CHILD(n, ports)), (DT_CHILD(DT_INST_CHILD(n, ports), port_##pid)), (DT_INST_CHILD(n, port_##pid)))

#define DT_INST_PORT_BY_ID(n, pid)                                                                 \
	COND_CODE_1(DT_NODE_EXISTS(_DT_INST_PORT_BY_ID(n, pid)), (_DT_INST_PORT_BY_ID(n, pid)), (DT_INST_CHILD(n, port)))

Also note that the 1st macro is not only an intermediate / internal macro. It can be used by drivers as well if they know exactly that the target port has a pid.

Same comment for the endpoint macros.

josuah · 2024-11-05T01:24:01Z

=> This does not work when there is no ports node but we still have multiple ports, each has a pid (they are not grouped under a ports node though). Note that the ports node is optional.

I had no idea! This all makes sense then.

josuah

All good for me! Thank you for your explanations on this.
+1 assuming that:

Indentation is tuned to get check_compliance.py happy
Documentation is added (glad to contribute it, but I do not want to break your workflow)
The naming matches what devicetree maintainers want
Their introduction in video.h is ok as per devicetree maintainers

Should the devicetree tag be added?

ngphibang · 2024-11-05T09:29:56Z

Relating to the remote endpoint and remote device macros:


> /* Handle the variability of "ports{port@0{}};" vs "port{};" while going up */
> #define DT_ENDPOINT_PARENT_DEVICE(node)                                                            \
> 	COND_CODE_1(DT_NODE_EXISTS(DT_CHILD(DT_GPARENT(node), port)),                              \
> 		    (DT_GPARENT(node)), (DT_PARENT(DT_GPARENT(node))))
> 
> /* Handle the "remote-endpoint-label" */
> #define DEVICE_DT_GET_REMOTE_DEVICE(node)                                                          \
> 	DEVICE_DT_GET(DT_ENDPOINT_PARENT_DEVICE(                                                   \
> 		DT_NODELABEL(DT_STRING_TOKEN(node, remote_endpoint_label))))

=> You check whether or not we have a port node (with no id) while we need to check rather on ports node because this will also break when we have multiple ports (with id) but no ports node.

However, by moving the node exist check to the local endpoint of the remote device, this version is simpler / better than mine. Will change to this. Thanks !

ngphibang · 2024-11-05T09:42:10Z

Their introduction in video.h is ok as per devicetree maintainers.

This is to be discussed. Ideally, it should be in devicetree.h / device.h. But remote-endpoint-label is not recognized system wide except in video-interfaces binding. Although we can declare this property in any dts without breaking the build but we cannot use the macros in code without having the corresponding binding. So, to introduce these macros in devicetree.h / device.h, we need to have also remote-endpoint-label in Zephyr generic bindings (base.yaml ?). How ?

If we keep them only for video, it's ok for the 1st phase. But do you know if there are examples in Zephyr that subsystem can have their own DT macros ? or all DT macros must go to devicetree.h ?

josuah · 2024-11-05T10:07:33Z

It looks like driver classes do have their own macros! In a subdirectory of include/zephyr/devicetree.

For instance include/zephyr/devicetree/video.h file with macro named like DT_VIDEO_... or DT_INST_VIDEO_... if following the same pattern.

zephyr/include/zephyr/devicetree.h

Lines 5146 to 5158 in 4bb9453

    
           /* have these last so they have access to all previously defined macros */ 
        
           #include <zephyr/devicetree/io-channels.h> 
        
           #include <zephyr/devicetree/clocks.h> 
        
           #include <zephyr/devicetree/gpio.h> 
        
           #include <zephyr/devicetree/spi.h> 
        
           #include <zephyr/devicetree/dma.h> 
        
           #include <zephyr/devicetree/pwms.h> 
        
           #include <zephyr/devicetree/fixed-partitions.h> 
        
           #include <zephyr/devicetree/ordinals.h> 
        
           #include <zephyr/devicetree/pinctrl.h> 
        
           #include <zephyr/devicetree/can.h> 
        
           #include <zephyr/devicetree/reset.h> 
        
           #include <zephyr/devicetree/mbox.h>

When the macros generate a struct specific to one subsystem (i.e. i2c_dt_spec), they are put in the include/zephyr/drivers/<class>.h with a name typically like <CLASS>_DT_...:

zephyr/include/zephyr/drivers/i2c.h

Lines 92 to 94 in 4bb9453

    
           #define I2C_DT_SPEC_GET_ON_I3C(node_id)					\ 
        
           	.bus = DEVICE_DT_GET(DT_BUS(node_id)),				\ 
        
           	.addr = DT_PROP_BY_IDX(node_id, reg, 0)

include/zephyr/drivers/video.h

danieldegrasse

General comment- we need additional documentation on these macros before we can move forwards with this PR, I assume that will be added?

ngphibang · 2024-11-07T13:37:01Z

General comment- we need additional documentation on these macros before we can move forwards with this PR, I assume that will be added?

Yes, I will put documentation and other things as discussed in the next push.

danieldegrasse

Looks fine to me, thank you for this update

ngphibang · 2024-12-05T17:27:20Z

Looks fine to me, thank you for this update

I still need to resolve @rruuaanng 's comments, will do soon :-).

ngphibang · 2024-12-05T21:24:57Z

@rruuaanng : In fact, there was already a blank line at the end of the file in my local IDE but github does not show it. I tried to add one more to the files as your request but then check-compliance has failed as you can see in CI check:

ERROR   : Test YAMLLint failed: 
YAMLLint (empty-lines):too many blank lines (1 > 0)
File:dts/bindings/test/vnd,video-multi-port.yaml
Line:15
Column:1
ERROR   : Test YAMLLint failed: 
YAMLLint (empty-lines):too many blank lines (1 > 0)
File:dts/bindings/test/vnd,video-single-port.yaml
Line:14
Column:1
ERROR   : Test Nits failed: 
Please remove blank lines at end of 'dts/bindings/test/vnd,video-multi-port.yaml'
ERROR   : Test Nits failed: 
Please remove blank lines at end of 'dts/bindings/test/vnd,video-single-port.yaml'
ERROR   : Test GitDiffCheck failed: 
0e805944c8829c236348b4dcdd41e4f9fb8eb6a0: dts/bindings/test/vnd,video-multi-port.yaml:15: new blank line at EOF.
0e805944c8829c236348b4dcdd41e4f9fb8eb6a0: dts/bindings/test/vnd,video-single-port.yaml:14: new blank line at EOF.

So, I will let it as before then ?

rruuaanng · 2024-12-06T00:34:56Z

@rruuaanng : In fact, there was already a blank line at the end of the file in my local IDE but github does not show it. I tried to add one more to the files as your request but then check-compliance has failed as you can see in CI check:


ERROR   : Test YAMLLint failed: 

YAMLLint (empty-lines):too many blank lines (1 > 0)

File:dts/bindings/test/vnd,video-multi-port.yaml

Line:15

Column:1

ERROR   : Test YAMLLint failed: 

YAMLLint (empty-lines):too many blank lines (1 > 0)

File:dts/bindings/test/vnd,video-single-port.yaml

Line:14

Column:1

ERROR   : Test Nits failed: 

Please remove blank lines at end of 'dts/bindings/test/vnd,video-multi-port.yaml'

ERROR   : Test Nits failed: 

Please remove blank lines at end of 'dts/bindings/test/vnd,video-single-port.yaml'

ERROR   : Test GitDiffCheck failed: 

0e805944c8829c236348b4dcdd41e4f9fb8eb6a0: dts/bindings/test/vnd,video-multi-port.yaml:15: new blank line at EOF.

0e805944c8829c236348b4dcdd41e4f9fb8eb6a0: dts/bindings/test/vnd,video-single-port.yaml:14: new blank line at EOF.

So, I will let it as before then ?

It seems that my comment is not so clear. If there is a blank line at the end of the file, you can ignore my comment. I just suggest that please forgive my unclear description.

Edit

Delete the extra blank line and keep only one blank line at the end of the file. I apologize again :(

ngphibang · 2024-12-06T07:45:31Z

@rruuaanng No problem :-). Done.

Add port and endpoint DT macros to retrieve the node id of the interested port/endpoint from its id. Also, add helpers to retrieve the peer remote device node from its local endpoint interface. Signed-off-by: Phi Bang Nguyen <[email protected]> Co-developed-by: Josuah Demangeon <[email protected]>

Add tests for the new port / endpoint DT macros Signed-off-by: Josuah Demangeon <[email protected]> Signed-off-by: Phi Bang Nguyen <[email protected]>

Drop the driver-defined macros to use the endpoint DT helpers instead. Signed-off-by: Phi Bang Nguyen <[email protected]>

ngphibang · 2024-12-06T10:11:50Z

force-push to fix CI build issue due to #79482 (which take these macros inside it) get merged first

ngphibang · 2024-12-06T16:12:02Z

@danieldegrasse Could you revisit ?

danieldegrasse

@decsny could you take a look as well?

ngphibang requested review from mmahadevan108 and dleach02 as code owners October 30, 2024 21:35

zephyrbot added platform: NXP Drivers NXP Semiconductors, drivers area: Video Video subsystem labels Oct 30, 2024

zephyrbot requested review from danieldegrasse, dbaluta, decsny, josuah, loicpoulain, manuargue and MarkWangChinese October 30, 2024 21:36

zephyrbot assigned dleach02 and mmahadevan108 Oct 30, 2024

ngphibang mentioned this pull request Oct 30, 2024

drivers: video: Introduce macro to deal with endpoints #73023

Closed

josuah requested changes Oct 31, 2024

View reviewed changes

josuah reviewed Nov 2, 2024

View reviewed changes

include/zephyr/drivers/video.h Outdated Show resolved Hide resolved

josuah mentioned this pull request Nov 2, 2024

video: emul: virtual driver for an imager and RX engine #79482

Merged

josuah previously approved these changes Nov 5, 2024

View reviewed changes

josuah mentioned this pull request Nov 5, 2024

Support circular DTS node references #57708

Open

danieldegrasse reviewed Nov 5, 2024

View reviewed changes

include/zephyr/drivers/video.h Outdated Show resolved Hide resolved

danieldegrasse reviewed Nov 5, 2024

View reviewed changes

ngphibang mentioned this pull request Nov 27, 2024

Implement video control framework #82158

Open

ngphibang dismissed josuah’s stale review via fbdfef0 November 29, 2024 00:26

josuah self-requested a review December 5, 2024 16:55

danieldegrasse previously approved these changes Dec 5, 2024

View reviewed changes

ngphibang dismissed danieldegrasse’s stale review via 4a12693 December 5, 2024 21:17

ngphibang force-pushed the add_endpoint_parser branch from b37117f to 4a12693 Compare December 5, 2024 21:17

ngphibang requested a review from rruuaanng December 5, 2024 22:00

josuah previously approved these changes Dec 5, 2024

View reviewed changes

rruuaanng approved these changes Dec 6, 2024

View reviewed changes

ngphibang dismissed josuah’s stale review via 9b4b9a7 December 6, 2024 07:37

ngphibang force-pushed the add_endpoint_parser branch from 4a12693 to 9b4b9a7 Compare December 6, 2024 07:37

ngphibang requested review from josuah, danieldegrasse and rruuaanng December 6, 2024 07:45

ngphibang and others added 3 commits December 6, 2024 11:06

tests: lib: devicetree: Add tests for endpoint DT macros

6c08072

Add tests for the new port / endpoint DT macros Signed-off-by: Josuah Demangeon <[email protected]> Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video: Use endpoint DT helpers

1ea737e

Drop the driver-defined macros to use the endpoint DT helpers instead. Signed-off-by: Phi Bang Nguyen <[email protected]>

ngphibang force-pushed the add_endpoint_parser branch from 9b4b9a7 to 1ea737e Compare December 6, 2024 10:07

rruuaanng approved these changes Dec 6, 2024

View reviewed changes

josuah approved these changes Dec 6, 2024

View reviewed changes

danieldegrasse approved these changes Dec 6, 2024

View reviewed changes

mmahadevan108 approved these changes Dec 6, 2024

View reviewed changes

kartben merged commit ab6141c into zephyrproject-rtos:main Dec 6, 2024
25 checks passed

This was referenced Dec 10, 2024

meta: Track progress on moving video drivers and devicetree to use the video-interfaces binding and the endpoint DT macros #80514

Open

usb: device_next: new USB Video Class (UVC) implementation #76798

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include: devicetree: Add port and endpoint macros #80649

include: devicetree: Add port and endpoint macros #80649

ngphibang commented Oct 30, 2024 •

edited

Loading

josuah left a comment

josuah Oct 31, 2024

danieldegrasse Nov 5, 2024

ngphibang Nov 7, 2024

josuah commented Nov 2, 2024 •

edited

Loading

ngphibang commented Nov 5, 2024 •

edited

Loading

josuah commented Nov 5, 2024

josuah left a comment

ngphibang commented Nov 5, 2024 •

edited

Loading

ngphibang commented Nov 5, 2024 •

edited

Loading

josuah commented Nov 5, 2024

danieldegrasse left a comment

ngphibang commented Nov 7, 2024

danieldegrasse left a comment

ngphibang commented Dec 5, 2024

ngphibang commented Dec 5, 2024 •

edited

Loading

rruuaanng commented Dec 6, 2024 •

edited

Loading

ngphibang commented Dec 6, 2024

ngphibang commented Dec 6, 2024

ngphibang commented Dec 6, 2024

danieldegrasse left a comment

		#define _DT_INST_PORT_BY_ID(n, pid) \
		COND_CODE_1(DT_NODE_EXISTS(DT_INST_CHILD(n, ports)), (DT_CHILD(DT_INST_CHILD(n, ports), port_##pid)), (DT_INST_CHILD(n, port_##pid)))

include: devicetree: Add port and endpoint macros #80649

include: devicetree: Add port and endpoint macros #80649

Conversation

ngphibang commented Oct 30, 2024 • edited Loading

josuah left a comment

Choose a reason for hiding this comment

josuah Oct 31, 2024

Choose a reason for hiding this comment

danieldegrasse Nov 5, 2024

Choose a reason for hiding this comment

ngphibang Nov 7, 2024

Choose a reason for hiding this comment

josuah commented Nov 2, 2024 • edited Loading

ngphibang commented Nov 5, 2024 • edited Loading

josuah commented Nov 5, 2024

josuah left a comment

Choose a reason for hiding this comment

ngphibang commented Nov 5, 2024 • edited Loading

ngphibang commented Nov 5, 2024 • edited Loading

josuah commented Nov 5, 2024

danieldegrasse left a comment

Choose a reason for hiding this comment

ngphibang commented Nov 7, 2024

danieldegrasse left a comment

Choose a reason for hiding this comment

ngphibang commented Dec 5, 2024

ngphibang commented Dec 5, 2024 • edited Loading

rruuaanng commented Dec 6, 2024 • edited Loading

ngphibang commented Dec 6, 2024

ngphibang commented Dec 6, 2024

ngphibang commented Dec 6, 2024

danieldegrasse left a comment

Choose a reason for hiding this comment

ngphibang commented Oct 30, 2024 •

edited

Loading

josuah commented Nov 2, 2024 •

edited

Loading

ngphibang commented Nov 5, 2024 •

edited

Loading

ngphibang commented Nov 5, 2024 •

edited

Loading

ngphibang commented Nov 5, 2024 •

edited

Loading

ngphibang commented Dec 5, 2024 •

edited

Loading

rruuaanng commented Dec 6, 2024 •

edited

Loading